Skip to content

feat(eval): add task analysis and trace publishing - #48

Draft
vincentkoc wants to merge 3 commits into
mainfrom
feat/task-analysis-export
Draft

feat(eval): add task analysis and trace publishing#48
vincentkoc wants to merge 3 commits into
mainfrom
feat/task-analysis-export

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

What does this PR do?

Adds reproducible task-level analysis and verified private trace publishing for
native ShellBench matrices.

The aggregate export now preserves the full experiment condition, reasoning
effort is treated as part of each matrix cell, and the analysis command
generates CSV, JSON, Markdown, and SVG diagnostics. A separate trace uploader
publishes immutable run archives and audit metadata to private S3 storage.

Why?

Run-level leaderboards can hide narrow task clusters that drive apparent model
or harness wins. The native exports also previously grouped default pair labels
by harness and model without reasoning effort, making low/medium/high runs easy
to blur together downstream.

Task revisions must remain separate. A 115-task archive and a 116-task archive
are different datasets, not extra repetitions of one matrix.

Trace publication also needs a repeatable path that does not place static
credentials in command arguments, repository files, or generated commands.

Changes

  • export harness, model, provider, reasoning, judge, task revision, and task
    suite metadata on every native run and task row
  • attach parent run score, coverage, exact passes, eligibility, and exclusion
    reason to each task row
  • include reasoning effort in generated pair labels
  • add clawbench task-analysis for:
    • task-cell summaries across repetitions
    • per-task variance rankings
    • paired harness task deltas
    • paired reasoning-effort task deltas
    • revision-aware JSON and Markdown summaries
    • SVG box-and-whisker plots
  • add clawbench trace-upload for:
    • final and checkpoint trace archives
    • trajectory validation reports and trace-gap audits
    • run-index provenance
    • SHA-256 object metadata and post-upload verification
    • resumable skips when object size and digest already match
    • a verified local and remote S3_UPLOAD_MANIFEST.json
  • use the standard AWS credential chain only; there are no access-key or
    secret-key CLI options
  • set no public ACL and use S3-managed encryption; destination bucket policy
    remains authoritative
  • add optional analysis and s3 dependencies
  • document both workflows using generic placeholders only

Existing aggregate CSVs need to be regenerated from their raw job directories
before analysis because historical files do not contain the new experiment
metadata.

Tests

  • python -m pytest -q (426 passed, 5 skipped)
  • python -m ruff check clawbench app.py scripts tests
  • secret, credential, bucket-name, URI, and personal-path diff scan
  • real-data analysis smoke against a 93-run, 31-cell, 116-task matrix
  • analysis output: 3,596 task-cell rows, 3,132 harness deltas, 2,204
    reasoning deltas, 116 task diagnostics, and four SVG plots
  • trace-upload dry-run against the canonical bundle: 104 files including
    all 93 immutable archives

@vincentkoc vincentkoc changed the title feat(analysis): add task matrix diagnostics feat(eval): add task analysis and trace publishing Jul 29, 2026
@vincentkoc vincentkoc mentioned this pull request Jul 29, 2026
Merged
3 tasks
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels Jul 29, 2026
@clawsweeper

clawsweeper Bot commented Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown

Codex review: found issues before merge. Reviewed August 2, 2026, 3:15 PM ET / 19:15 UTC.

ClawSweeper review

What this changes

This PR adds richer native evaluation exports, a task-level analysis command and reports, and an S3 command to publish trace archives with checksum manifests.

Merge readiness

⛔ Blocked by patch quality or review findings - 12 items remain

Keep this draft PR open. Its task-analysis and trace-publication capabilities are not present on current main, but the unchanged head still has the prior P1 privacy and aggregate-label compatibility blockers; as a MEMBER-authored PR, it also requires explicit maintainer handling rather than automated cleanup.

Priority: P1
Reviewed head: 5de5016b954b8755239536536bf771e46a497c76
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The PR has substantial stated validation and a focused feature goal, but two unresolved P1 correctness and security issues make it not merge-ready.
Proof confidence 🦐 gold shrimp (3/6) Not applicable: Not applicable as a contributor proof gate because the author association is MEMBER; the PR body reports real-data analysis and a trace-upload dry run, but no linked artifact was available for independent inspection in this review.
Patch quality 🦪 silver shellfish (2/6) Security review found an item that needs attention.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: Not applicable as a contributor proof gate because the author association is MEMBER; the PR body reports real-data analysis and a trace-upload dry run, but no linked artifact was available for independent inspection in this review.
Evidence reviewed 6 items Current main lacks the proposed commands: Current main has no task-analysis or trace-upload modules or corresponding tests, so the PR's central functionality is not already implemented.
Current aggregate label remains the legacy contract: Current main derives a default pair label as harness-model. The PR changes that fallback to include reasoning effort, which changes a field already exported to downstream aggregate consumers.
Current native-evaluation ownership: The current aggregate path was introduced and most recently maintained through native evaluation commits by Vincent Koc, including the base commit for this PR's review surface.
Findings 3 actionable findings [P1] Verify bucket privacy before uploading traces
[P1] Preserve the aggregate pair-label compatibility contract
[P2] Align the private-upload documentation with enforcement
Security Needs attention Upload can proceed to public or unverifiable buckets: Bucket reachability does not establish that a bucket policy, ACL configuration, or public-access setting prevents public reads, so the uploader can violate its private-trace contract before checksum verification completes.

How this fits together

ShellBench's native evaluation runner writes aggregate and per-task results from benchmark run directories. This PR extends those exports into task diagnostics and sends raw trace archives plus audit metadata to an S3 destination for later research review.

flowchart LR
  A[Native benchmark runs] --> B[Aggregate CSV exports]
  B --> C[Task analysis command]
  C --> D[CSV JSON Markdown and SVG reports]
  A --> E[Trace archives and audit files]
  E --> F[Trace upload command]
  F --> G[Private S3 objects and manifest]
Loading

Decision needed

Question Recommendation
Should ShellBench support trace uploads only when it can verify that the destination bucket is non-public, failing closed when the caller lacks the permissions needed to establish that fact? Fail closed on unverifiable privacy: Require non-public bucket policy status and public-access protections before any upload, and reject destinations that cannot be verified.

Why: The PR promises private trace publication, but enforcing that promise requires an explicit security boundary for buckets whose policy or public-access configuration cannot be inspected.

Before merge

  • Verify bucket privacy before uploading traces (P1) - head_bucket proves only that the configured identity can reach the bucket. The following upload path can send raw trace archives even when bucket policy or public-access settings permit public reads, contradicting the documented private-publication contract. Check effective public-access controls and policy status before upload, and fail closed when they cannot be verified.
  • Preserve the aggregate pair-label compatibility contract (P1) - Adding reasoning effort to the fallback pair_label changes existing harness-model grouping keys for consumers of aggregate CSVs. Keep the legacy label and export a separate full experiment key, or provide an explicit versioned migration with focused compatibility coverage.
  • Align the private-upload documentation with enforcement (P2) - The new README promises private S3 publication while saying bucket policy is authoritative, but the implementation only establishes bucket reachability. Once enforcement is added, document the exact required bucket checks and the fail-closed behavior; otherwise the documented safety claim is misleading.
  • Resolve security concern: Upload can proceed to public or unverifiable buckets - Bucket reachability does not establish that a bucket policy, ACL configuration, or public-access setting prevents public reads, so the uploader can violate its private-trace contract before checksum verification completes.
  • Resolve merge risk (P1) - Trace archives can contain benchmark trajectories and audit material; uploading after only bucket reachability verification can disclose them if the destination is public or its effective public status cannot be verified.
  • Resolve merge risk (P1) - Changing the default aggregate pair_label from harness-model to harness-model-reasoning can silently split or invalidate existing downstream grouping and comparison logic.
  • Resolve merge risk (P1) - GitHub reports the draft branch as dirty, so the final merge result needs a rebase and review refresh after the functional blockers are fixed.
  • Complete next step (P1) - A MEMBER-authored draft needs maintainer confirmation of the fail-closed S3 privacy boundary and review of the compatibility-preserving export design before it can proceed.
  • Improve patch quality - Fail closed on public or unverifiable S3 destinations and add focused coverage.
  • Improve patch quality - Preserve existing aggregate pair labels while exposing reasoning as a separate condition dimension.
  • Improve patch quality - Update the S3 documentation to match the enforced privacy contract and refresh the branch against current main.

Findings

  • [P1] Verify bucket privacy before uploading traces — clawbench/trace_upload.py:140
  • [P1] Preserve the aggregate pair-label compatibility contract — scripts/native_eval/aggregate.py:442
  • [P2] Align the private-upload documentation with enforcement — README.md:469
  • [high] Upload can proceed to public or unverifiable buckets — clawbench/trace_upload.py:140
Agent review details

Security

Needs attention: The new S3 uploader can handle sensitive trace archives without verifying that the configured bucket is effectively private.

Review metrics

Metric Value Why it matters
Patch scope 10 files affected; 1,585 additions The PR combines data-export compatibility, analysis reporting, dependencies, documentation, and a new storage command in one review surface.
Operator commands 2 new CLI commands task-analysis and trace-upload introduce separate local-analysis and remote-storage workflows with different safety requirements.

Merge-risk options

Maintainer options:

  1. Enforce destination privacy before upload (recommended)
    Fail closed before any archive upload unless the bucket's public-access block and policy status establish a non-public destination, with focused tests for public and unverifiable buckets.
  2. Preserve the existing pair-label contract
    Keep pair_label as the existing harness-model value and export reasoning effort through a new condition field or versioned migration path.
  3. Pause the publishing feature
    Keep the task-analysis work separate and defer trace upload until maintainers choose and document the repository's enforceable private-storage contract.
Copy recommended automerge instruction 
@clawsweeper automerge

Special instructions:
Fail closed before upload unless the destination's public-access block and policy status prove it is non-public; add focused tests for public and unverifiable buckets.

Technical review

Best possible solution:

Keep the legacy aggregate pair label stable while adding a separate condition identifier, and make trace publishing fail closed unless the destination's effective public-access controls and policy status prove it is private.

Do we have a high-confidence way to reproduce the issue?

Not applicable: this PR proposes new evaluation and trace-publication capabilities rather than reporting a pre-existing broken behavior. Current main confirms the commands do not yet exist.

Is this the best way to solve the issue?

No. Task analysis is a plausible addition, but the uploader must enforce its private-storage promise and the aggregate export must retain compatibility before this is a maintainable solution.

Full review comments:

  • [P1] Verify bucket privacy before uploading traces — clawbench/trace_upload.py:140
    head_bucket proves only that the configured identity can reach the bucket. The following upload path can send raw trace archives even when bucket policy or public-access settings permit public reads, contradicting the documented private-publication contract. Check effective public-access controls and policy status before upload, and fail closed when they cannot be verified.
    Confidence: 0.95
  • [P1] Preserve the aggregate pair-label compatibility contract — scripts/native_eval/aggregate.py:442
    Adding reasoning effort to the fallback pair_label changes existing harness-model grouping keys for consumers of aggregate CSVs. Keep the legacy label and export a separate full experiment key, or provide an explicit versioned migration with focused compatibility coverage.
    Confidence: 0.93
  • [P2] Align the private-upload documentation with enforcement — README.md:469
    The new README promises private S3 publication while saying bucket policy is authoritative, but the implementation only establishes bucket reachability. Once enforcement is added, document the exact required bucket checks and the fail-closed behavior; otherwise the documented safety claim is misleading.
    Confidence: 0.87

Overall correctness: patch is incorrect
Overall confidence: 0.91

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning high; reviewed against 884dd1bb5511.

Labels

Label changes:

  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: Not applicable as a contributor proof gate because the author association is MEMBER; the PR body reports real-data analysis and a trace-upload dry run, but no linked artifact was available for independent inspection in this review.
  • remove status: 📣 needs proof: Current PR status label is status: ⏳ waiting on author.

Label justifications:

  • P1: The trace uploader can publish sensitive benchmark archives to an insufficiently verified destination, and the aggregate-label change can break existing research consumers.
  • merge-risk: 🚨 compatibility: The PR changes the default exported pair_label shape used to group aggregate benchmark results.
  • merge-risk: 🚨 security-boundary: The proposed trace-upload path handles potentially sensitive archives without proving that the S3 destination is non-public.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦐 gold shrimp and patch quality is 🦪 silver shellfish.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: Not applicable as a contributor proof gate because the author association is MEMBER; the PR body reports real-data analysis and a trace-upload dry run, but no linked artifact was available for independent inspection in this review.

Evidence

Security concerns:

  • [high] Upload can proceed to public or unverifiable buckets — clawbench/trace_upload.py:140
    Bucket reachability does not establish that a bucket policy, ACL configuration, or public-access setting prevents public reads, so the uploader can violate its private-trace contract before checksum verification completes.
    Confidence: 0.95

What I checked:

  • Current main lacks the proposed commands: Current main has no task-analysis or trace-upload modules or corresponding tests, so the PR's central functionality is not already implemented. (clawbench/cli.py:1, 884dd1bb5511)
  • Current aggregate label remains the legacy contract: Current main derives a default pair label as harness-model. The PR changes that fallback to include reasoning effort, which changes a field already exported to downstream aggregate consumers. (scripts/native_eval/aggregate.py:424, 884dd1bb5511)
  • Current native-evaluation ownership: The current aggregate path was introduced and most recently maintained through native evaluation commits by Vincent Koc, including the base commit for this PR's review surface. (scripts/native_eval/aggregate.py:424, 9afe6a4f1746)
  • Previously reported blockers remain on the same head: The supplied review context records the same head SHA as the previous completed review cycle and lists bucket-privacy enforcement, pair-label compatibility, and documentation alignment as unresolved findings. (clawbench/trace_upload.py:140, 5de5016b954b)
  • Current-main provenance: Current main is at a later native-evaluation commit, but the proposed task-analysis and S3 upload surfaces are absent; the PR base is an ancestor of current main rather than evidence that this feature landed. (scripts/native_eval/aggregate.py:85, 884dd1bb5511)
  • Inspection limitation: The checkout could read current-main files and history, but its partial clone could not retrieve the PR-head blob because the configured GitHub remote was unreachable. The supplied PR diff and prior review context were used for the head-specific findings.

Likely related people:

  • vincentkoc: Current-main history shows Vincent Koc authored the native matrix runner, its later stability work, and the parity/eligibility change forming this PR's base surface. (role: native evaluation feature owner and recent area contributor; confidence: high; commits: 69f75c6629c4, b9acd9f7a010, 9afe6a4f1746; files: scripts/native_eval/aggregate.py, clawbench/cli.py, README.md)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (14 earlier review cycles; latest 8 shown)
  • reviewed 2026-07-31T19:46:07.974Z sha 5de5016 :: found issues before merge. :: [P1] Verify effective bucket privacy before uploading traces
  • reviewed 2026-08-01T00:26:36.124Z sha 5de5016 :: found issues before merge. :: [P1] Verify effective bucket privacy before uploading traces
  • reviewed 2026-08-01T03:04:49.584Z sha 5de5016 :: found issues before merge. :: [P1] Verify effective bucket privacy before uploading traces
  • reviewed 2026-08-01T06:13:21.326Z sha 5de5016 :: needs changes before merge. :: [P1] Verify effective bucket privacy before uploading traces
  • reviewed 2026-08-01T10:56:24.673Z sha 5de5016 :: needs changes before merge. :: [P1] Verify effective bucket privacy before uploading traces | [P2] Honor the documented private S3 destination contract
  • reviewed 2026-08-01T12:21:17.069Z sha 5de5016 :: found issues before merge. :: [P1] Verify effective bucket privacy before uploading traces | [P2] Align the private-upload documentation with enforcement
  • reviewed 2026-08-01T17:56:41.383Z sha 5de5016 :: found issues before merge. :: [P1] Verify effective bucket privacy before uploading traces | [P1] Preserve the aggregate pair-label compatibility contract | [P2] Align the private-upload documentation with enforcement
  • reviewed 2026-08-02T13:17:04.174Z sha 5de5016 :: found issues before merge. :: [P1] Verify bucket privacy before uploading traces | [P1] Preserve the aggregate pair-label compatibility contract | [P2] Align the private-upload documentation with enforcement

@clawsweeper clawsweeper Bot added status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 Urgent regression or broken agent/channel workflow affecting real users now. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jul 30, 2026
@clawsweeper clawsweeper Bot added status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Aug 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@vincentkoc